-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Replace ethers Event with Log #738
Conversation
The ethers Event type is an extension of the underlying Log type, which is actually the type that contains almost all of the event data that the SDK needs for Across. Since Event extends Log, it's safe to narrow the scope of the RPC data down to a Log type. This significantly reduces the delta to viem's Log type and is a key stepping stone for viem and ethers to be used interchangeably for RPC interfacing & transport. It additionally seems like this simplification _might_ also support Solana events (...with some eventual coercion).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm - imo this should be a minor bump because it may break the FE + relayer + indexer.
Ah - good call! It should definitely bump the minor. Bumped to 3.2.0 here: 361a9b5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
I've tested the indexer linking this version and there are no issues.
The ethers Event type is an extension of the underlying Log type, which is actually the type that contains almost all of the event data that the SDK needs for Across. Since Event extends Log, it's safe to narrow the scope of the RPC data down to a Log type. This significantly reduces the delta to viem's Log type and is a key stepping stone for viem and ethers to be used interchangeably for RPC interfacing & transport.
It additionally seems like this simplification might also support Solana events (...with some eventual coercion).
It should be noted that this change touches some code that makes a lot of assumptions about the event types that are being handled, and there are some warts. I've tried not to make things any less safe than they already are, but this code is generally in need of a refactoring to improve type safety.